Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: on error, reset spawnedProcess #41033

Merged
merged 1 commit into from Jan 24, 2024
Merged

fix: on error, reset spawnedProcess #41033

merged 1 commit into from Jan 24, 2024

Conversation

maikelohcfg
Copy link
Contributor

Description of Change

For squirrel update in case of errors during the update the exit event listener may not be called resulting on spawnedProcess remains defined in memory preventing further updates checks to run with success.

Checklist

  • PR description included and stakeholders cc'd

Release Notes

Notes: none

@welcome Welcome
Copy link

welcome bot commented Jan 17, 2024

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

  • fix: don't overwrite prevent_default if default wasn't prevented
  • feat: add app.isPackaged() method
  • docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

  • Follow the JavaScript, C++, and Python coding style.
  • Run npm run lint locally to catch formatting errors earlier.
  • Document any user-facing changes you've made following the documentation styleguide.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jan 17, 2024
@maikelohcfg maikelohcfg changed the title reset spawnedProcess instance in case of error fix: reset spawnedProcess instance in case of error Jan 17, 2024
@maikelohcfg maikelohcfg marked this pull request as draft January 19, 2024 16:06
@maikelohcfg maikelohcfg reopened this Jan 19, 2024
@maikelohcfg maikelohcfg marked this pull request as ready for review January 19, 2024 16:09
ckerr
ckerr approved these changes Jan 22, 2024
View reviewed changes
Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks reasonable to me. Definitely valid to clear out spawnedProcess and spawnedArgs to make future calls possible after recovering from an error.

I was wondering about the potential for reject() to be called twice, once each for the error and exit signals, so in case any other reviewers were curious -- this is safe. After a promise is resolved, subsequent calls to a promise resolve function are no-ops.

@ckerr ckerr added component/auto-updater semver/patch backwards-compatible bug fixes labels Jan 22, 2024
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jan 22, 2024
@ckerr ckerr added new-pr 🌱 PR opened in the last 24 hours target/26-x-y PR should also be added to the "26-x-y" branch. target/27-x-y PR should also be added to the "27-x-y" branch. target/28-x-y PR should also be added to the "28-x-y" branch. target/29-x-y PR should also be added to the "29-x-y" branch. labels Jan 22, 2024
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jan 22, 2024
@ckerr ckerr changed the title fix: reset spawnedProcess instance in case of error fix: on error, reset spawnedProcess Jan 24, 2024
@ckerr ckerr merged commit a05bfd3 into electron:main Jan 24, 2024
34 checks passed
@welcome Welcome
Copy link

welcome bot commented Jan 24, 2024

Congrats on merging your first pull request! 🎉🎉🎉

@release-clerk Release Clerk
Copy link

release-clerk bot commented Jan 24, 2024

No Release Notes

@trop
Copy link
Contributor

trop bot commented Jan 24, 2024

I was unable to backport this PR to "26-x-y" cleanly;
you will need to perform this backport manually.

@trop trop bot removed the target/26-x-y PR should also be added to the "26-x-y" branch. label Jan 24, 2024
@trop
Copy link
Contributor

trop bot commented Jan 24, 2024

I was unable to backport this PR to "27-x-y" cleanly;
you will need to perform this backport manually.

@trop trop bot added needs-manual-bp/26-x-y needs-manual-bp/27-x-y and removed target/27-x-y PR should also be added to the "27-x-y" branch. labels Jan 24, 2024
@trop
Copy link
Contributor

trop bot commented Jan 24, 2024

I was unable to backport this PR to "28-x-y" cleanly;
you will need to perform this backport manually.

@trop trop bot removed the target/28-x-y PR should also be added to the "28-x-y" branch. label Jan 24, 2024
@trop trop bot mentioned this pull request Jan 24, 2024
Merged
@trop
Copy link
Contributor

trop bot commented Jan 24, 2024

I have automatically backported this PR to "29-x-y", please check out #41110

@trop trop bot added in-flight/29-x-y and removed target/29-x-y PR should also be added to the "29-x-y" branch. labels Jan 24, 2024
ckerr pushed a commit that referenced this pull request Jan 24, 2024
@maikelohcfg @ckerr
fix: on error, reset spawnedProcess (#41033)
108dd63 
reset spawnedProcess instance in case of error
@ckerr ckerr mentioned this pull request Jan 24, 2024
Merged
3 tasks
@trop
Copy link
Contributor

trop bot commented Jan 24, 2024

@ckerr has manually backported this PR to "28-x-y", please check out #41111

@trop trop bot added in-flight/28-x-y merged/29-x-y PR was merged to the "29-x-y" branch. and removed needs-manual-bp/28-x-y in-flight/29-x-y labels Jan 24, 2024
zcbenz pushed a commit that referenced this pull request Jan 25, 2024
@ckerr @maikelohcfg
fix: on error, reset spawnedProcess (#41111)
33057eb 
fix: on error, reset spawnedProcess (#41033)

reset spawnedProcess instance in case of error

Co-authored-by: Maikel Ortega Hernández <maikeloh@gmail.com>
@trop trop bot added merged/28-x-y PR was merged to the "28-x-y" branch. and removed in-flight/28-x-y labels Jan 25, 2024
@maikelohcfg maikelohcfg deleted the squirrel-update branch January 25, 2024 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ckerr ckerr ckerr approved these changes

Assignees
No one assigned
Labels
component/auto-updater merged/28-x-y PR was merged to the "28-x-y" branch. merged/29-x-y PR was merged to the "29-x-y" branch. semver/patch backwards-compatible bug fixes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@maikelohcfg @ckerr @jkleinsc